Security hardening, performance fixes, dependency updates, and PHPUnit 12 readiness#2006
Open
grasmash wants to merge 19 commits into
Open
Security hardening, performance fixes, dependency updates, and PHPUnit 12 readiness#2006grasmash wants to merge 19 commits into
grasmash wants to merge 19 commits into
Conversation
… thecodingmachine/safe Loosens the exact thecodingmachine/safe pin to ^3.4 now that the de-aliased 3.x line is stable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PHPStan 2 uses 50-70% less memory and unlocks levels up to 10. Analysis still passes at the configured level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No Laminas code is referenced anywhere in src/, tests/, or bin/. The package remains installed transitively via ltd-beget/dns-zone-configurator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Symfony Console ships completion for bash/zsh/fish out of the box, but it was undocumented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Hoist getSkippedApiCommands() out of the per-endpoint loop - Replace O(n^2) namespace visibility scan in generateApiListCommands() with a single-pass keyed map Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Redact key/secret/password/token values before sending command arguments and options to Amplitude telemetry, and extend the Bugsnag context redaction beyond --password to --key and --secret - Use StrictHostKeyChecking=accept-new instead of =no for all SSH, rsync, and git operations so changed host keys fail instead of being silently accepted (TOFU; OpenSSH 7.6+) - Pass browser launch URIs as process arguments instead of a shell string, eliminating shell injection via crafted URIs - chmod credential files written by JsonDataStore to 0600 - Enforce 0600 on generated SSH private keys and 0700 on ~/.ssh - Replace error suppression in posix_isatty and aliases archive extraction with explicit error handling and actionable messages - Remove unused CommandBase::$cloudApplication property; use strict array comparison in CommandBase Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces @group, @dataProvider, @Covers, @coversDefaultClass, and @requires annotations with native PHP attributes across 56 test files. Doc-comment metadata is deprecated in PHPUnit 11 and removed in PHPUnit 12; this eliminates all 305 deprecation notices from the test run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
checkForNewVersion() previously hit the GitHub releases API on every command invocation, adding network latency to all commands and risking GitHub rate limits. Cache the result per installed version for 24 hours, and clear it via self:clear-caches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Gate the Windows 'start' rewrite in startBrowser() behind an OS check so a literal 'start' browser on Linux/macOS is not rewritten to cmd.exe - Recurse into nested array values in redactSensitiveData() so sensitive keys inside array-typed arguments are also redacted - Sanitize the update-check cache key with an allowlist regex so version strings with reserved cache characters (e.g. 1.0.0+meta) cannot throw Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2006 +/- ##
============================================
+ Coverage 92.45% 92.49% +0.04%
- Complexity 1960 1991 +31
============================================
Files 123 123
Lines 7114 7232 +118
============================================
+ Hits 6577 6689 +112
- Misses 537 543 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Guard isTtyStream() with function_exists('posix_isatty') so the new
test and the helper work on Windows, where the posix extension is
absent
- Strengthen ApiCommandHelper list-command tests to assert namespace,
aliases, description, multi-namespace output, and continue-not-break
iteration
- Add a normal-verbosity git clone test pinning the verbosity comparison
- Assert the update-check cache is cleared by self:clear-caches and that
no upgrade message shows when the CLI is up to date
- Ignore cache-TTL mutations (expiresAfter) in infection, which are not
observable without manipulating the clock
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
grasmash
added
enhancement
dependencies
chmod() is a no-op on Windows, so the 0600/0700 assertions added for credential and SSH key hardening cannot hold there. Restrict those tests to linux|darwin, matching the existing convention for OS-specific tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2006/acli.phar |
Every acli invocation used to instantiate and fully configure all ~485 spec-derived api:* and acsf:* commands up front, and getApiCommands() loaded the entire ~1 MB Cloud API spec into memory to do it. Only one command ever runs. Register those commands through a Symfony FactoryCommandLoader instead: - Registration is driven by a lightweight per-spec manifest (command name, path/method, visibility flags) cached separately from the full spec — ~90 KB vs ~1 MB — so invoking a non-API command (the common case) no longer loads or parses the full spec at all. - Each command's full definition is built by a closure only when that command is actually requested. - The full spec load is memoized per process so building many commands (e.g. for 'list') parses it at most once. For a typical non-API command this cuts command registration from ~30 ms to ~10 ms and roughly halves peak memory (~33 MB to ~16 MB). 'acli list' output, per-command --help, and api:list are byte-for-byte unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The new manifest-building logic is served from a checksum-keyed cache, so its covering tests passed against a manifest built by unmutated code, letting mutations in buildApiSpecManifest survive on CI (where the cache was warm). Bypass the spec cache in the factory tests so the manifest is rebuilt from source under test, and add a direct buildApiSpecManifest test with a crafted spec that pins the continue-not-break behavior for ignored methods. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
I'm no longer on the devx team, @anujkaushal can find someone to review this |
Contributor
Author
|
@anujkaushal ^^ |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Acquia CLI’s security posture, reduces startup cost for common (non-API) invocations by moving spec-derived commands to lazy registration, updates dependencies/tooling, and modernizes PHPUnit metadata to attributes for PHPUnit 12 readiness.
Changes:
- Lazily register
api:*/acsf:*commands via a cached manifest +FactoryCommandLoader, and improve Cloud API spec caching/memoization. - Security hardening: redact sensitive telemetry payload values, use
StrictHostKeyChecking=accept-new, avoid shell-interpreting browser URIs, and enforce restrictive credential/key file permissions. - Testing & DX: convert PHPUnit annotations to attributes, add targeted tests for caching/hardening behaviors, update docs and issue templates, and bump dev dependencies.
Reviewed changes
Copilot reviewed 79 out of 81 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/src/Misc/TelemetryHelperTest.php | Converts PHPUnit metadata to attributes; adds redaction unit tests. |
| tests/phpunit/src/Misc/LocalMachineHelperTest.php | Adds tests for non-shell URI handling, multi-word browser commands, and TTY warning handling. |
| tests/phpunit/src/Misc/ExceptionListenerTest.php | Converts data provider to attribute. |
| tests/phpunit/src/Misc/ChecklistTest.php | Converts serial-group metadata to attribute. |
| tests/phpunit/src/DataStore/JsonDataStoreTest.php | New test ensuring JsonDataStore writes files with 0600 perms (Unix). |
| tests/phpunit/src/Commands/UpdateCommandTest.php | Adds tests for cached update checks and “up-to-date” messaging; adjusts mocking. |
| tests/phpunit/src/Commands/Ssh/SshKeyUploadCommandTest.php | Converts data provider to attribute. |
| tests/phpunit/src/Commands/Ssh/SshKeyCreateCommandTest.php | Converts data provider to attribute; adds secure perms enforcement test (Unix). |
| tests/phpunit/src/Commands/Self/TelemetryCommandTest.php | Adds test ensuring telemetry redacts sensitive option values; converts groups/providers to attributes. |
| tests/phpunit/src/Commands/Self/ClearCacheCommandTest.php | Ensures both alias and update-check caches are cleared; converts group to attribute. |
| tests/phpunit/src/Commands/Remote/SshCommandTest.php | Updates SSH args expectation to StrictHostKeyChecking=accept-new; converts group to attribute. |
| tests/phpunit/src/Commands/Remote/DrushCommandTest.php | Updates SSH args expectation to StrictHostKeyChecking=accept-new; converts provider to attribute. |
| tests/phpunit/src/Commands/Remote/AliasesDownloadCommandTest.php | Converts providers/OS requirement to attributes; updates extraction error expectation. |
| tests/phpunit/src/Commands/Push/PushFilesCommandTest.php | Updates rsync ssh option to accept-new. |
| tests/phpunit/src/Commands/Push/PushDatabaseCommandTest.php | Converts provider to attribute; updates rsync/ssh expectations to accept-new. |
| tests/phpunit/src/Commands/Push/PushArtifactCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/Pull/PullDatabaseCommandTest.php | Converts provider to attribute; removes flaky sleep by disabling redraw throttle. |
| tests/phpunit/src/Commands/Pull/PullCommandTestBase.php | Updates rsync ssh option to accept-new. |
| tests/phpunit/src/Commands/Pull/PullCodeCommandTest.php | Adds verbosity/output-forwarding behavior test; converts provider to attribute; updates git SSH option to accept-new. |
| tests/phpunit/src/Commands/Ide/Wizard/IdeWizardCreateSshKeyCommandTest.php | Converts groups/OS requirement to attributes. |
| tests/phpunit/src/Commands/Ide/IdeXdebugToggleCommandTest.php | Converts providers to attributes. |
| tests/phpunit/src/Commands/Ide/IdeServiceStopCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Ide/IdeServiceStartCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Ide/IdeServiceRestartCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Ide/IdePhpVersionCommandTest.php | Converts providers to attributes. |
| tests/phpunit/src/Commands/Ide/IdeOpenCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Ide/IdeListCommandTest.php | Converts groups to attributes. |
| tests/phpunit/src/Commands/Ide/IdeInfoCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Ide/IdeCreateCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/Env/EnvDeleteCommandTest.php | Converts providers/groups to attributes. |
| tests/phpunit/src/Commands/Env/EnvCreateCommandTest.php | Converts providers/groups to attributes. |
| tests/phpunit/src/Commands/DocsCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/CommandBaseTest.php | Converts providers/groups to attributes. |
| tests/phpunit/src/Commands/CodeStudio/CodeStudioWizardCommandTest.php | Converts providers/groups/OS requirement to attributes. |
| tests/phpunit/src/Commands/CodeStudio/CodeStudioPipelinesMigrateCommandTest.php | Converts provider/group/OS requirement to attributes. |
| tests/phpunit/src/Commands/CodeStudio/CodeStudioPhpVersionCommandTest.php | Converts provider/group to attributes. |
| tests/phpunit/src/Commands/Auth/AuthLoginCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/TaskWaitCommandTest.php | Converts providers to attributes. |
| tests/phpunit/src/Commands/App/NewFromDrupal7CommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/NewCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/LinkCommandTest.php | Converts group to attribute. |
| tests/phpunit/src/Commands/App/From/RecommendationsTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/From/ProjectBuilderTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/From/DefinedRecommendationTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/From/ConfigurationTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/App/From/AbandonmentRecommendationTest.php | Converts covers metadata to attribute (removes docblock covers). |
| tests/phpunit/src/Commands/App/AppVcsInfoTest.php | Converts groups to attributes. |
| tests/phpunit/src/Commands/Api/ApiCommandTest.php | Converts providers/groups to attributes. |
| tests/phpunit/src/Commands/Api/ApiCommandHelperTest.php | Adds tests for manifest/lazy factories/spec caching/list command behavior. |
| tests/phpunit/src/Commands/Acsf/AcsfAuthLogoutCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/Commands/Acsf/AcsfAuthLoginCommandTest.php | Converts providers/OS requirement to attributes. |
| tests/phpunit/src/Commands/Acsf/AcsfApiCommandTest.php | Converts provider to attribute. |
| tests/phpunit/src/CloudApi/PathRewriteConnectorTest.php | Converts providers and covers metadata to attributes. |
| tests/phpunit/src/CloudApi/ConnectorFactoryTest.php | Converts provider and covers metadata to attributes. |
| tests/phpunit/src/CloudApi/ClientServiceTest.php | Converts provider to attribute. |
| tests/phpunit/src/CloudApi/AcsfClientServiceTest.php | Converts provider to attribute. |
| tests/phpunit/src/Application/KernelTest.php | Converts serial-group metadata to attribute. |
| tests/phpunit/src/Application/HelpApplicationTest.php | Converts serial-group metadata to attributes. |
| tests/phpunit/src/Application/ExceptionApplicationTest.php | Converts serial-group metadata to attribute. |
| tests/phpunit/src/Application/ComposerScriptsListenerTest.php | Converts serial-group metadata to attributes. |
| tests/phpunit/src/AcsfApi/AcsfServiceTest.php | Converts provider to attribute. |
| src/Helpers/TelemetryHelper.php | Adds recursive sensitive-data redaction and expands context redaction. |
| src/Helpers/SshHelper.php | Harden SSH args to StrictHostKeyChecking=accept-new. |
| src/Helpers/LocalMachineHelper.php | Removes shell-string browser invocation; adds explicit TTY detection helper. |
| src/DataStore/JsonDataStore.php | Enforces 0600 permissions for stored JSON config data. |
| src/Command/Ssh/SshKeyCommandBase.php | Enforces restrictive perms (0600/0700) after key generation. |
| src/Command/Self/ClearCacheCommand.php | Clears update-check cache in addition to alias cache. |
| src/Command/Remote/AliasesDownloadCommand.php | Replaces error suppression with explicit error handling on archive extraction. |
| src/Command/Push/PushDatabaseCommand.php | Harden rsync SSH option to accept-new. |
| src/Command/Pull/PullCommandBase.php | Harden git SSH option to accept-new; pins verbosity comparison behavior. |
| src/Command/CommandBase.php | Redacts telemetry args/options; adds 24h cached update check via FilesystemAdapter. |
| src/Command/Api/ApiCommandHelper.php | Adds spec memoization, fallback caching, lazy factory map generation, and manifest building for faster startup. |
| README.md | Documents shell completion usage. |
| infection.json5 | Ignores cache TTL mutations that aren’t clock-testable. |
| composer.lock | Updates locked dependency versions for toolchain/security/perf work. |
| composer.json | Updates dependency constraints and removes unused dependency. |
| CLAUDE.md | Adds contributor/architecture/conventions notes for the repo. |
| bin/acli | Switches spec-derived command registration to lazy FactoryCommandLoader. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds feature request issue template. |
| .github/ISSUE_TEMPLATE/config.yml | Adds issue template config + security/support contact links. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds bug report issue template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+527
to
+532
| $manifest = $this->buildApiSpecManifest($this->getCloudApiSpec($specFilePath)); | ||
| $cache->warmUp([ | ||
| $cacheKey => $manifest, | ||
| $cacheKey . '.checksum' => $checksum, | ||
| ]); | ||
| return $manifest; |
Comment on lines
+429
to
+440
| // Split the browser command on whitespace to support commands | ||
| // with arguments (e.g. "open -a Firefox") while passing the URI | ||
| // as a single argument so it is never shell-interpreted. | ||
| $cmd = array_values(array_filter(explode(' ', $browser), static function (string $part): bool { | ||
| return $part !== ''; | ||
| })); | ||
| if ($cmd[0] === 'start' && OsInfo::isWindows()) { | ||
| // On Windows, `start` is a cmd.exe built-in rather than an | ||
| // executable, and it treats the first quoted argument as a | ||
| // window title, so pass an empty title before the URI. | ||
| $cmd = ['cmd', '/c', 'start', '']; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Headline: measurable wins
The performance numbers come from not loading the full ~1 MB Cloud API spec on every invocation.
acli list, per-command--help, andapi:listoutput are byte-for-byte unchanged.Performance
api:*/acsf:*commands and load the entire Cloud API spec into memory, though only one command ever runs. They are now registered through a SymfonyFactoryCommandLoaderdriven by a lightweight ~90 KB manifest cached separately from the spec, so running a non-API command (the common case) never loads or parses the full spec. Each command's definition is built only when requested; the full spec load is memoized per process.ApiCommandHelper: hoisted a constant lookup out of the per-endpoint loop and replaced an O(n²) namespace-visibility scan with a single-pass map.Hardening
StrictHostKeyChecking=accept-newinstead of=nofor ssh/rsync/git operations.Dependencies
thecodingmachine/safe→ ^3.4).laminas/laminas-validator.composer audit: no known CVEs.typhonius/acquia-logstream), PHP_CodeSniffer 4 (acquia/coding-standards,drupal/coder).Testing & DX
sleep(1)inPullDatabaseCommandTest; +30 tests covering the hardening, caching, and lazy-loading changes.Test plan
composer unit— 586 tests, 0 failures, 0 deprecationscomposer stan— no errorscomposer cs/ GrumPHP — cleanacli list,api:list, and per-command--help🤖 Generated with Claude Code